Skip to content

when tonumber() is recorded as nil, specialize to the failing ctype#1

Open
javierguerragiraldez wants to merge 1 commit intolua-users-foundation:v2.1from
javierguerragiraldez:ffi_tonumber
Open

when tonumber() is recorded as nil, specialize to the failing ctype#1
javierguerragiraldez wants to merge 1 commit intolua-users-foundation:v2.1from
javierguerragiraldez:ffi_tonumber

Conversation

@javierguerragiraldez
Copy link
Member

@javierguerragiraldez javierguerragiraldez commented Apr 6, 2018

bug report LuaJIT#408. a function was traced where a tonumber() couldn't convert a struct, so it returned constant nil (and optimized away). When called again with an int cdata, it just executed without checking the ctype.

this patch keeps the ctype guard even in the nil case.

@javierguerragiraldez
Copy link
Member Author

javierguerragiraldez commented Apr 6, 2018

testcase:

local ffi = require 'ffi'
local function isnumberlike(x)
	local is
	for i=1,100 do
		is = tonumber(x)
	end
	return not not is
end

print("isnumberlike(struct)?", isnumberlike(ffi.new('struct {}')))
print("isnumberlike(int64_t)?", isnumberlike(9LL))

expected:
isnumberlike(struct)? false
isnumberlike(int64_t)? true

@mraleph
Copy link

mraleph commented Apr 8, 2018

LGTM

@javierguerragiraldez javierguerragiraldez added the bug Something isn't working label Apr 8, 2018
@mraleph mraleph self-requested a review April 9, 2018 10:58
kostja
kostja suggested changes Apr 11, 2018
View reviewed changes
src/lj_crecord.c
J->base[0] = crec_ct_tv(J, d, 0, J->base[0], &rd->argv[0]);
} else {
/* specialize to the ctype that couldn't be converted */
argv2cdata(J, J->base[0], &rd->argv[0]);
Copy link

@kostja kostja Apr 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about adding a test case?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mraleph mraleph mraleph approved these changes

+1 more reviewer

@kostja kostja kostja requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

bug Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@javierguerragiraldez @mraleph @kostja